Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool for checking the status of a Cloud Run service. However, the current implementation contains several critical issues, including compilation errors due to incorrect function signatures and type mismatches, a malformed JSON struct tag, and an incomplete tool registration function that would prevent the new tool from working at all. I have provided detailed comments and code suggestions to address these problems and ensure the new functionality is correctly implemented.
| func addGetServiceStatusTool(server *mcp.Server, crClient cloudrunclient.CloudRunClient) { | ||
| getServiceStatusToolFunc = func(ctx context.Context, req* mcp.CallToolRequest, args GetServiceStatusArgs){ | ||
| state, err := crClient.GetServiceStatus(ctx, args.ProjectID, args.Location, args.ServiceName) | ||
| if err != nil { | ||
| return &mcp.CallToolResult{}, nil, fmt.Errorf("failed to get service state: %w", err) | ||
| } | ||
| return &mcp.CallToolResult{}, state, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
This new function to add the service status tool is incomplete and has several issues that will cause compilation and runtime errors:
- Function Name Mismatch: The function is named
addGetServiceStatusTool, but it's called asaddGetServiceStatusin theRegistermethod. This will cause an "undefined" compile error. - Missing Tool Registration: The function defines a tool handler (
getServiceStatusToolFunc) but never registers it with the MCP server usingmcp.AddTool. The tool will not be available. - Incorrect Handler Signature: The function literal for the handler is missing its return types
(*mcp.CallToolResult, any, error). This is a compile error. - Undeclared Variable: The variable
getServiceStatusToolFuncis used without being declared at the package level. You need to add avardeclaration for it, similar tolistServicesToolFunc. - Formatting Issues: There are several formatting problems, including incorrect indentation and a missing space in
req* mcp.CallToolRequest.
Here is a corrected version of the function. Please note you will also need to add var getServiceStatusToolFunc func(ctx context.Context, req *mcp.CallToolRequest, args GetServiceStatusArgs) (*mcp.CallToolResult, any, error) at the package level.
| func addGetServiceStatusTool(server *mcp.Server, crClient cloudrunclient.CloudRunClient) { | |
| getServiceStatusToolFunc = func(ctx context.Context, req* mcp.CallToolRequest, args GetServiceStatusArgs){ | |
| state, err := crClient.GetServiceStatus(ctx, args.ProjectID, args.Location, args.ServiceName) | |
| if err != nil { | |
| return &mcp.CallToolResult{}, nil, fmt.Errorf("failed to get service state: %w", err) | |
| } | |
| return &mcp.CallToolResult{}, state, nil | |
| } | |
| } | |
| func addGetServiceStatus(server *mcp.Server, crClient cloudrunclient.CloudRunClient) { | |
| getServiceStatusToolFunc = func(ctx context.Context, req *mcp.CallToolRequest, args GetServiceStatusArgs) (*mcp.CallToolResult, any, error) { | |
| state, err := crClient.GetStatus(ctx, args.ProjectID, args.Location, args.ServiceName) | |
| if err != nil { | |
| return &mcp.CallToolResult{}, nil, fmt.Errorf("failed to get service state: %w", err) | |
| } | |
| return &mcp.CallToolResult{}, state, nil | |
| } | |
| mcp.AddTool(server, &mcp.Tool{Name: "cloudrun.service_status", Description: "Provide detailed status of a Cloud Run service."}, getServiceStatusToolFunc) | |
| } |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool, cloudrun.service_status, to fetch the status of a Cloud Run service. The implementation is straightforward, but I've identified a critical issue that could lead to a panic due to a potential nil pointer dereference. I've also pointed out a code formatting issue. Additionally, the new functionality lacks corresponding unit tests. Please add tests for the GetServiceStatus tool to ensure its correctness and prevent future regressions.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
cloudrun.service_status: Provide detailed status of a Cloud Run services and logs from the service.